The problem also happens when the array is converted from packed to hash:

class C {
    function __destruct() {
        global $arr;
        // array is converted from packed to hash. in_hash->arPacked becomes invalid.
        $arr["str"] = 0;
    }
}

$arr = ["1", new C, "2"];

array_splice($arr, 1, 2);

Or if the array is released entirely:

class C {
    function __destruct() {
        global $arr;
        $arr = null;
    }
}

$arr = ["1", new C, "2"];

array_splice($arr, 1, 2);

An alternative fix would be to increase the refcount of in_hash before processing it. CoW will ensure that the array can not be modified.

I tried by using GC_ADDREF, but then calling the underlying zend_hash_packed_del_val() fails because it asserts HT_ASSERT_RC1. Am I missing something?

I missed that the function is also mutating in_hash. This gives us two possible solutions:

• Avoid mutating in_hash in php_splice(), so that we can GC_ADDREF() in_hash. The purpose of the mutations seems to be to release the removed values themselves, but also to help with maintenance of iterator positions.
• Or delay releasing the removed values until the end of php_splice(), so that side effects happen when it doesn't matter anymore.

The first solution would be faster, but is like more complex.

Or instead of making it complicated, use a little hack:

diff --git a/ext/standard/array.c b/ext/standard/array.c
index a2a0459ec3b..08b0f5253d6 100644
--- a/ext/standard/array.c
+++ b/ext/standard/array.c
@@ -3214,6 +3214,10 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
 	zval		*entry;				/* Hash entry */
 	uint32_t    iter_pos = zend_hash_iterators_lower_pos(in_hash, 0);
 
+	/* Enforce separation on running user code */
+	GC_ADDREF(in_hash);
+	HT_ALLOW_COW_VIOLATION(in_hash); /* will be reset down below when setting the flags for in_hash */
+
 	/* Get number of entries in the input hash */
 	num_in = zend_hash_num_elements(in_hash);
 
@@ -3372,18 +3376,23 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
 	HT_SET_ITERATORS_COUNT(&out_hash, HT_ITERATORS_COUNT(in_hash));
 	HT_SET_ITERATORS_COUNT(in_hash, 0);
 	in_hash->pDestructor = NULL;
-	zend_hash_destroy(in_hash);
-
-	HT_FLAGS(in_hash)          = HT_FLAGS(&out_hash);
-	in_hash->nTableSize        = out_hash.nTableSize;
-	in_hash->nTableMask        = out_hash.nTableMask;
-	in_hash->nNumUsed          = out_hash.nNumUsed;
-	in_hash->nNumOfElements    = out_hash.nNumOfElements;
-	in_hash->nNextFreeElement  = out_hash.nNextFreeElement;
-	in_hash->arData            = out_hash.arData;
-	in_hash->pDestructor       = out_hash.pDestructor;
-
-	zend_hash_internal_pointer_reset(in_hash);
+	if (UNEXPECTED(GC_DELREF(in_hash) == 0)) {
+		zend_array_destroy(in_hash);
+		zend_hash_destroy(&out_hash);
+	} else {
+		zend_hash_destroy(in_hash);
+
+		HT_FLAGS(in_hash)          = HT_FLAGS(&out_hash);
+		in_hash->nTableSize        = out_hash.nTableSize;
+		in_hash->nTableMask        = out_hash.nTableMask;
+		in_hash->nNumUsed          = out_hash.nNumUsed;
+		in_hash->nNumOfElements    = out_hash.nNumOfElements;
+		in_hash->nNextFreeElement  = out_hash.nNextFreeElement;
+		in_hash->arData            = out_hash.arData;
+		in_hash->pDestructor       = out_hash.pDestructor;
+
+		zend_hash_internal_pointer_reset(in_hash);
+	}
 }
 /* }}} */
 

Isn't it risky to use HT_ALLOW_COW_VIOLATION? It's seems defined as an empty macro when ZEND_DEBUG is false?

Also, I tried your implementation but I think there's something wrong with this test case:

--TEST--
GH-16649: array_splice UAF with destructor adding element to array
--FILE--
<?php
class C {
    function __destruct() {
        global $arr;
        $arr[] = 0;
    }
}

$arr = ["1", new C, "2"];

array_splice($arr, 1, 2);
var_dump($arr);
?>
--EXPECT--
array(3) {
  [0]=>
  string(1) "1"
  [3]=>
  int(0)
}

It fails with the following output:

array(3) {
  [0]=>
  string(1) "1"
  [2]=>
  string(1) "2" <---- this shouldn't be here
  [3]=>
  int(0)
}

If I'm right, the "2" should not appear after the splice?

Isn't it risky to use HT_ALLOW_COW_VIOLATION?

The array is being protected by having RC>1. So I think it's fine as it can't "escape".

It's seems defined as an empty macro when ZEND_DEBUG is false?

Yes, because RC1 assertions for arrays are only checked in debug mode.

Good observation about the test. It happens because the reassignment to in_hash is skipped. There's a couple of different approach we could take, one could be to simply throw an exception when the original array has disappeared; this may be the simplest solution as this can only happen with malicious code.

Given the convoluted code required to reproduce this case, it seems to me to be a viable solution.

I implemented your solution @nielsdos. I wonder if the message should avoid mentioning destructors and be less specific in case it happens at an unexpected place yet to be detected?

Oh, I now also see why the code uses zend_hash_destroy() rather than zend_array_destroy(), it's to keep the HashTable allocation alive. I'm afraid you'll have to revert my suggestion. Sorry!

Yes I just spotted that with the CI. No worries 🙂